SDK | Upgrade AWS SDK to v3 - Block Store S3 #9067
Conversation
|
""" WalkthroughThis change migrates S3-related functionality from AWS SDK v2 to AWS SDK v3. It updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant cloud_utils.js
participant AWS STS
participant AWS S3
Caller->>cloud_utils.js: createSTSS3SDKv3Client(params, additionalParams)
cloud_utils.js->>cloud_utils.js: generate_aws_sdkv3_sts_creds(params, roleSessionName)
cloud_utils.js->>AWS STS: AssumeRoleWithWebIdentity
AWS STS-->>cloud_utils.js: Credentials
cloud_utils.js->>cloud_utils.js: Instantiate S3 client with credentials
cloud_utils.js-->>Caller: S3 client instance
Caller->>AWS S3: S3 operations using v3 client
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
naveenpaul1
force-pushed
the
block_store_ser_sdkv3
branch
5 times, most recently
from
e811dd7 to
c9c3fde
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/agent/block_store_services/block_store_s3.js(14 hunks)src/test/unit_tests/test_cloud_utils.js(3 hunks)src/util/cloud_utils.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/**/*.*`: Ensure that the PR includes tests for the changes.
src/test/**/*.*: Ensure that the PR includes tests for the changes.
src/test/unit_tests/test_cloud_utils.js
🔇 Additional comments (13)
package.json (1)
85-85: Verify if aws-sdk v2 is still needed.The old AWS SDK v2 dependency is still present. If this upgrade is intended to remove v2 usage from the Block Store S3 component, verify if this dependency is still needed elsewhere in the codebase.
#!/bin/bash # Description: Check if aws-sdk v2 is still used in the codebase # Expect: If only used in files not related to this PR, it's safe to keep echo "Checking aws-sdk v2 usage in the codebase:" rg -A 3 "require\('aws-sdk'\)" --type js echo -e "\nChecking AWS.S3 usage:" rg -A 3 "AWS\.S3" --type js echo -e "\nChecking new AWS SDK v3 usage:" rg -A 3 "@aws-sdk/" --type jssrc/test/unit_tests/test_cloud_utils.js (4)
7-7: LGTM: Import correctly updated for SDK v3.The import has been properly updated to use the modular AWS SDK v3 client.
25-32: LGTM: Stubbing correctly updated for SDK v3.The stubbing approach is correctly updated to mock the
sendmethod onSTSClient.prototype, which is the proper way to stub SDK v3 client calls.
49-49: Verify the timeout extension rationale.The timeout was extended from the default to 50 seconds. Ensure this is necessary for STS operations or if there are specific performance considerations.
59-63: LGTM: Correctly handles asynchronous credential access.The test properly uses
awaitto access credentials and region from the S3 client configuration, which is required for SDK v3.src/agent/block_store_services/block_store_s3.js (5)
5-5: LGTM: Import correctly updated for SDK v3.The import has been properly updated to use the modular AWS SDK v3 S3 client.
15-15: LGTM: Added proper HTTP handler import.The NodeHttpHandler import is correctly added for SDK v3 HTTP configuration.
43-54: LGTM: S3 client instantiation correctly updated for SDK v3.The S3 client configuration is properly updated with:
- Credentials passed as an object
forcePathStyleinstead ofs3ForcePathStyle- Proper
requestHandlerconfiguration withNodeHttpHandler
61-72: LGTM: S3 compatible endpoint configuration updated correctly.The configuration for S3-compatible endpoints is properly updated with:
applyChecksumreplacing the deprecateds3DisableBodySigning- Proper HTTP agent configuration through
requestHandler
82-85: LGTM: S3 method calls correctly updated for SDK v3.All S3 method calls have been properly updated by removing the
.promise()chaining, which is correct since SDK v3 methods return promises natively.Also applies to: 109-112, 167-170, 194-199, 241-248, 265-268, 309-315, 325-328, 345-350, 353-358, 389-396, 424-427
src/util/cloud_utils.js (3)
13-14: LGTM: Imports correctly updated for SDK v3.The imports have been properly updated to include the modular AWS SDK v3 STS client and commands, along with the NodeHttpHandler.
36-44: LGTM: S3 client creation correctly updated for SDK v3.The
createSTSS3Clientfunction is properly updated with:
- New S3 client from SDK v3
- Proper
requestHandlerconfigurationforcePathStyleinstead of the deprecateds3ForcePathStyle
49-67: LGTM: STS credential generation correctly updated for SDK v3.The
generate_aws_sts_credsfunction is properly updated with:
- STSClient instantiation with region configuration
- AssumeRoleWithWebIdentityCommand usage
- Proper credential extraction from the response
- Maintained error handling and validation
naveenpaul1
force-pushed
the
block_store_ser_sdkv3
branch
3 times, most recently
from
139e101 to
80d4fc2
Compare
|
after this PR, STS request in namespace_s3 is failing, Will update the Namespce_s3 SDK version aslo. |
naveenpaul1
force-pushed
the
block_store_ser_sdkv3
branch
3 times, most recently
from
f68832b to
a1cda69
Compare
naveenpaul1
force-pushed
the
block_store_ser_sdkv3
branch
3 times, most recently
from
24e3bec to
6b79454
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/agent/block_store_services/block_store_s3.js(17 hunks)src/test/unit_tests/util_functions_tests/test_cloud_utils.js(2 hunks)src/util/cloud_utils.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/util/cloud_utils.js
- src/agent/block_store_services/block_store_s3.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
src/test/unit_tests/util_functions_tests/test_cloud_utils.js (3)
10-11: LGTM! Correct imports for AWS SDK v3 testing.The imports are appropriate for testing AWS SDK v3 functionality -
STSClientfor STS operations andfsfor file system stubbing.
19-19: Good practice to use a constant for the region.This avoids hardcoding the region value and improves maintainability.
110-125: LGTM! Proper async credential handling for AWS SDK v3.The test correctly handles the async nature of credentials in AWS SDK v3 by using
awaitfors3.config.credentials()ands3.config.region()calls. This is the correct approach for testing v3 client configuration.
Signed-off-by: naveenpaul1 <napaul@redhat.com> Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
block_store_ser_sdkv3
branch
from
6b79454 to
9f61f6c
Compare
Describe the Problem
Upgrade AWS SDK to v3 - Block Store S3
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Chores